-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove global metadata cache, refactor custom_metadata API #238
Conversation
Codecov Report
@@ Coverage Diff @@
## main #238 +/- ##
==========================================
+ Coverage 83.79% 85.81% +2.02%
==========================================
Files 26 26
Lines 3259 3265 +6
==========================================
+ Hits 2731 2802 +71
+ Misses 528 463 -65
Continue to review full report at Codecov.
|
Flipping out of draft as tests pass and docs are updated (unless I missed anything). Before merging, should we decide on:
We also could do this in a separate PR before tagging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exciting! Didn't know about Base.ImmutableDict
, cool.
Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; thanks for taking this on! I left one suggestion to simplify toiddict
.
Co-authored-by: Jacob Quinn <quinn.jacobd@gmail.com>
From Slack convo: I'll merge this PR once CI passes and can open a follow-up later to delete the deprecated regions before we actually tag the v2.0 release. |
Hmm. Looks like Julia 1.3 tests now fail because the and the Julia 1.6 tests fail because julia> foldl(Base.ImmutableDict, ("a" => "b",))
"a" => "b" I think I'll just revert back to the original more verbose implementation, which also has that slight performance benefit anyway |
essentially implements the approach described here #90 (comment)
I haven't run tests fully locally yet so may still have some lingering bugs.They pass 😎I also haven't been super performance-conscious here so we should be careful to make sure regressions aren't being introduced.
Since we're doing a major bump for this anyway, I wonder whether it's a good time to fully delete some of the lingering deprecations here.
closes #90, #211